Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tealdeer: rustls -> native-tls #346497

Closed
wants to merge 1 commit into from
Closed

Conversation

newAM
Copy link
Member

@newAM newAM commented Oct 4, 2024

Things done

The recent tealdeer v1.7.0 release added support for the native-tls crate which dynamically links libssl on Linux instead of the rustls crate.

This change reduces the tldr binary size:

  • x86_64-linux: 2097k -> 1433k (assuming libssl already exists for other purposes)
  • aarch64-darwin: 3620k -> 2620k
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@emilazy
Copy link
Member

emilazy commented Oct 5, 2024

It seems like Rustls is the upstream default and what they build their own releases with. I think sticking with the memory‐safe implementation makes more sense without a strong reason to diverge from that.

Comment on lines 24 to 28
cargoBuildFlags = [
# use OpenSSL for the TLS backend instead of the default rustls
"--no-default-features"
"--features=native-tls"
];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cargoBuildFlags = [
# use OpenSSL for the TLS backend instead of the default rustls
"--no-default-features"
"--features=native-tls"
];
buildNoDefaultFeatures = true;
buildFeatures = [ "native-tls" ];

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Updated.

@newAM
Copy link
Member Author

newAM commented Oct 6, 2024

It seems like Rustls is the upstream default and what they build their own releases with. I think sticking with the memory‐safe implementation makes more sense without a strong reason to diverge from that.

I do not think it's fair to call rustls memory safe, it depends on the ring crate which is largely ASM on x86_64 for performance reasons.

The rationale is that the binary sizes are substantially smaller with libssl, which is almost always already on the system since nix itself links it. Measurements are in the original post.

@emilazy
Copy link
Member

emilazy commented Oct 6, 2024

Well, it’s memory safe in the sense that all Rust is: the bulk of the code involving gnarly and vulnerability‐prone stuff like X.509 parsing etc. is written in a memory‐safe dialect, but at the bottom there are of course primitives that need to be trusted. Cryptography primitives are usually not especially high‐risk for memory safety, since they tend to operate on fixed blocks with known bounds. I don’t think that shaving off a megabyte is worth the sacrifice here without another reason to diverge from upstream to use something less safe.

@h7x4
Copy link
Member

h7x4 commented Oct 6, 2024

Would a flag in the package args be a possible solution? There's still the need to agree on whether it should be opt-in or opt-out, but at least it would leave the choice in the hands of the users who care enough to look for it

@emilazy
Copy link
Member

emilazy commented Oct 6, 2024

If there’s a meaningful functionality difference for users (e.g. more protocol versions supported with OpenSSL?), then that could be reasonable. I don’t think we should just delegate every packaging decision to users, though, especially ones trading off memory safety for a megabyte of storage.

The upstream change that added native-tls support justifies it with support for architectures like PowerPC and MIPS. We can’t compile Rust code for MIPS at present. Maybe we can for PowerPC? I’m not sure if there are any NixOS‐on‐PowerPC users (it’s certainly not a platform we support in any official capacity).

@Aleksanaa
Copy link
Member

Aleksanaa commented Oct 29, 2024

I think it makes more sense to stick to the upstream default when it doesn't break functionality. If you really care about this, you can add an option to achieve it.

@newAM
Copy link
Member Author

newAM commented Nov 16, 2024

I'm happy to accept the consensus that it's better off with rustls, thanks everyone for the discussion!

@newAM newAM closed this Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants